feat: render and time a real basemap in the browser gate, and emit MapLibre's worker (#63) - #314
Conversation
#63's browser gate proved the routing to a basemap archive and not the picture, because there was no archive to render: #53 publishes one and is blocked on #52, which is Phase 3. This builds a PMTiles v3 archive from arithmetic instead, emits it into the harness build, and renders from it. What that immediately found is a defect in shipping code. MapLibre v6 parses every vector tile in a Web Worker and locates it with `new URL('./maplibre-gl-worker.mjs', import.meta.url)`, which under a bundler resolves against the hashed chunk MapLibre landed in — a file no bundler emits, because the expression is built from a variable. The script 404s, the Worker is constructed anyway, every tile-parse message goes into it unanswered, and a production map fetches all its tiles and draws none of them with no error anywhere. `maplibre.ts` now calls `setWorkerUrl` with a `?worker&url` import; `?url` alone would not do, because the dist worker imports its sibling `maplibre-gl-shared.mjs`. The cold-load measurement #63's eighth criterion asks for is taken and printed on every run, and is explicitly only the client-side floor: served from the loopback interface, it carries none of the hosting latency that criterion is pointed at. That half still belongs to #53. - apps/web/browser/pmtiles-fixture.ts: the archive, written from the v3 spec and MVT 2.1; no OpenStreetMap data, so no attribution attaches to it - apps/web/browser/harness.ts: reads the drawing buffer each frame and times the first painted tile, with `preserveDrawingBuffer` forced by the harness - apps/web/browser/map.browser.spec.ts: the render, its control, and the number - apps/web/vite.browser.config.ts: emits the archive at build time; never committed, and at a path that leaves the "#53 has published nothing" tripwire asserting its 404 - apps/web/vitest.config.ts: includes `browser/**/*.test.ts`, so the fixture's own unit test is run by something Refs #63 Refs #53 Signed-off-by: Matthew Cronin <mgcronin@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016X4EwacF1KuXDPhXzunifS Signed-off-by: mcronin <mgcronin@gmail.com>
Criterion 8, second data point — the CI runnerThe number in the body was taken on a developer's laptop. Run 35046425989, Both together, which is the more useful reading than either alone:
So the honest reading of the floor is: three sequential round trips before the first tile paints, whatever the host costs per round trip. On a store ADR 0010 D-1 records as 500 ms or higher, that is the figure #53's measurement will be dominated by, and it is a structural cost this PR can report but not reduce. 🤖 Generated with Claude Code |
mgcronin
left a comment
There was a problem hiding this comment.
Code Review — PR #314
Recommendation: merge with follow-ups — 0 blocking findings, 6 non-blocking.
This is automated analysis, not a human sign-off. The authenticated gh identity is also the PR author, so GitHub cannot record an independent approval or change request from it and this is published as a comment. A human still owns the merge decision.
What I re-ran, on this branch, at Node 24.20.0 / pnpm 11.18.0 / Chromium 1243
| Gate | Result |
|---|---|
pnpm install --frozen-lockfile |
already up to date |
pnpm run check:repo |
115 + 16 + 21 + 28 passed |
pnpm run format:check |
clean |
pnpm run lint |
clean |
pnpm run typecheck |
clean, all 7 packages |
pnpm run build |
clean — emits dist/assets/maplibre-gl-worker-CL9gHS1m.js (485.85 kB) |
pnpm run check:wiring |
40 watched files, 270 production modules, all reachable |
pnpm run check:a11y-suite |
17 files, all selected |
pnpm --filter @onyourleft/web run test |
129 files, 2105 tests passed |
pnpm run test:browser |
74 passed — matches the claim (was 71) |
gh pr checks 314 |
Repository rules pass, CodeQL pass |
The production defect is real, and the fix is load-bearing — verified, not accepted
I applied M8 myself on a clean tree (setWorkerUrl(workerUrl) commented out, nothing else) and re-ran the map spec:
1) paints the archive’s own tiles under the ride trace — criterion 1, and 7
Error: no basemap colour reached the drawing buffer. Looked for #eeece7, #b9d4e8, #ffffff;
read #f5f3ef ×10 over 300 frames
2) records what a cold load costs the client — criterion 8, as far as it goes
expect(load.painted).toBe(true) -> false
2 failed, 7 passed
Exactly the two fixture tests red; the six pre-existing browser tests and the control green. That is the claim in the body, reproduced. I also confirmed the mechanism from the shipped bytes rather than from the write-up: maplibre-gl.mjs builds its worker URL as new URL(`./${t}`, e).href — a template with a variable, so not statically analysable — and maplibre-gl-worker.mjs's first line is an import of its sibling, which is why ?worker&url and not ?url. setWorkerUrl is an exported, documented API in maplibre-gl.d.ts. In pnpm run build the emitted worker is referenced only from assets/maplibre-C01SpLki.js, the lazy map chunk — the code split is genuinely unaffected.
I spot-checked two more mutations on a clean tree and restored byte-identically:
- M1 (geometry as raw bytes, not packed varints) → only draws its polygons the way round that makes them exterior rings red, 9 passed. As described, including that it is the winding assertion that catches it.
- M4 (offset column without
+1) → only answers for every tile from zoom zero to its declared maximum red, 9 passed.
The mutation list is credible.
Requirements: PASS for what is claimed
Refs #63 rather than Closes #63 is correct under CLAUDE.md §7 — criterion 8's hosted half needs #53 → #52, and the ADR 0010 open-question reference would have been orphaned. Commit carries Signed-off-by:. No dependency added, removed or bumped; pnpm-lock.yaml and every manifest untouched. docs/adr/* untouched (protected path). No changelog convention exists here, correctly none added.
Security: CLEAN
No user input reaches the one shipping change; its argument is a bundler-emitted, same-origin constant. ?archive= and ?paintDeadline= live only in apps/web/browser/, which the second Vite config keeps out of the shipped bundle — I confirmed apps/web/dist contains no basemap-fixture.pmtiles and no harness entry. The new protobuf/PMTiles encoder runs only in a Vitest file and a build script, over bytes this repository generates. Criterion 3 is now executed on a page where tiles actually flowed, and the newly-appearing worker request is inside that assertion's reach.
Credit where it is due
Three things stand out. The fixture is served at /basemap-fixture.pmtiles rather than /basemap.pmtiles, so the #53 tripwire stays falsifiable instead of being discharged by a file of our own — that is the harder and the right call. The control page (paints no tile colour at all when the archive is missing) plus M11 is what stops this whole gate being green because nothing was measured, and it is the reason I believe the positive assertion. And FIXTURE_SOURCE_LAYERS being asserted equal to the style's own source-layer set means a future style layer fails here rather than silently going uncovered.
Non-blocking findings
Six, all inline below except the last, which is on a line this PR does not touch:
(6) INFO, pre-existing — not introduced here. CLAUDE.md:1286 still reads "There are four entries today, not two: the map, the game, the HUD and the capture tool." There are five in vite.browser.config.ts — shell arrived with #307. This PR edits §4f heavily, so carrying the one-word fix here would be cheap; equally fair to leave it for whoever next touches §4f.
Scope of this review
Read all ten changed files in full, plus apps/web/src/map/basemap.test.ts, playwright.config.ts, vite.config.ts and the relevant maplibre-gl dist bytes. Ran the gates in the table above. Did not run check:capacitor, check:licences or test:coverage locally — no dependency or Capacitor surface changed and CI ran all three green. Did not exercise a hosted archive, which is the point of the PR's own caveat.
| * requirement that the root lie inside the first 16,384 bytes — so a client can | ||
| * prefetch header and directory in one range request — is met by a very wide | ||
| * margin. That prefetch is the reason the cold-load number below is two round | ||
| * trips rather than three. |
There was a problem hiding this comment.
LOW (documentation) — high confidence. This sentence is the claim the PR body says was falsified and corrected, left standing in the one file a reader is pointed at for provenance.
That prefetch is the reason the cold-load number below is two round
trips rather than three.
The run reports three archive range requests (I reproduced it: 3 archive range request(s) costing 3.2 ms in total), and map.browser.spec.ts:390-400 explicitly records that "a first draft of this comment asserted 'two round trips, no more'; the run reported three". SharedPromiseCache caches headers and directories, not tiles, so it is one request per visible tile — which the PR body itself calls the reusable finding for #53.
Secondary: "the cold-load number below" points at nothing in this file; the number is printed by the spec.
This matters more here than it would elsewhere, because CLAUDE.md's own convention is that a sentence which has become false is a defect rather than untidiness — and pmtiles-fixture.ts's header is where §4f now sends a reader before quoting the number.
| * From immediately before `renderer.create` to the frame that painted. | ||
| * | ||
| * The **client's** share of a cold load: a range request for the header and | ||
| * root directory, a range request for the tile, the MVT decode, and the first |
There was a problem hiding this comment.
LOW (documentation) — high confidence. The same two-request framing as pmtiles-fixture.ts:487, one layer down:
The client's share of a cold load: a range request for the header and
root directory, a range request for the tile, the MVT decode, and the first
draw.
Measured, it is a range request for the header and root directory and then one per visible tile — three on the run this gate performs. Since archiveRequests is published right beside firstPaintMs precisely so the hosted number can be decomposed, the doc comment describing the decomposition is the one place it is worth being exact. Suggest "…and a range request per visible tile (three on this gate's viewport — SharedPromiseCache caches headers and directories, not tiles)".
| * this module is only evaluated when a map is actually wanted — `main.tsx` | ||
| * reaches it through `import()`. | ||
| */ | ||
| setWorkerUrl(workerUrl); |
There was a problem hiding this comment.
Verified rather than accepted: I reverted only this line on an otherwise clean tree and the two fixture tests went red with read #f5f3ef ×10 over 300 frames while all six pre-existing browser tests and the control stayed green. The defect and the fix are both real, and the reasoning about ?worker&url vs ?url checks out against the installed maplibre-gl-worker.mjs, whose first statement is an import of maplibre-gl-shared.mjs. Good find, and the note above it is the right length for what it costs to rediscover.
INFO / follow-up, non-blocking. One consequence worth writing down somewhere durable: the only thing that can catch a regression here builds with vite.browser.config.ts, not with the app's own vite.config.ts. A change to base, build.assetsDir or worker.format in vite.config.ts could break the shipped worker URL while the harness build stays green, and apps/web/browser/** is outside the coverage report so nothing else would notice either. I checked by hand on this branch — pnpm run build emits assets/maplibre-gl-worker-CL9gHS1m.js, referenced only from the lazy assets/maplibre-*.js chunk, so today it is correct. A one-line assertion over the built dist (the worker asset exists and is named by the map chunk) would close it, or a sentence in docs/architecture.md §"The map dependencies" saying which config the gate actually builds.
| // The archive is named in the URL rather than in the code, which is | ||
| // criterion 7 — *"the basemap URL is configuration, and a test proves the | ||
| // map renders against a second archive URL without a code change"* — | ||
| // executed by a real engine rather than by a style comparison. Nothing in |
There was a problem hiding this comment.
INFO — medium confidence, a wording point rather than a defect. The title says "criterion 1, and 7" and this comment reads criterion 7 as executed here. What the harness actually varies is its own ?archive= switch: harness.ts:27-36 records that it constructs a BasemapConfig directly and deliberately bypasses readBasemapConfig, because that function refuses a non-https: URL. The product's configuration path — VITE_BASEMAP_PMTILES_URL → readBasemapConfig → basemapStyle — is covered in basemap.test.ts:88 ("renders against a second archive with no code change — criterion 7") and is not exercised on this page.
That is a perfectly good additional execution of the claim in a real engine, and it is worth having. It is just worth one clause saying which half it is, so a later reader does not take this as the config reader being proved end to end — the same care the file already takes over styleOrigins not being subsumed.
| expect(walk(roads?.features[0]?.commands ?? []).length).toBe(2); | ||
| // And it is not closed: a `ClosePath` here would make MapLibre read a | ||
| // degenerate ring rather than a line. | ||
| expect(roads?.features[0]?.commands).not.toContain(15); |
There was a problem hiding this comment.
nit: not.toContain(15) is asserted over the raw command and parameter stream, so it would also fail if a future geometry produced a parameter varint that happened to equal 15 (zigzag 15 is a delta of −8). Today's road line encodes [9, 127, 4096, 10, 8448, 0], so it cannot happen — but the assertion reads as "no ClosePath" and is not quite that.
walk() already separates commands from parameters; asserting on the command ids it steps over would say the intended thing exactly. Entirely optional.
| if (first === undefined) { | ||
| throw new RangeError('a line needs at least one vertex'); | ||
| } | ||
| commands.push(9, zigzag(first.x), zigzag(first.y)); |
There was a problem hiding this comment.
nit: line() writes its first vertex as zigzag(first.x) where ring() (line 289) writes zigzag(first.x - cursorX) with the cursor at zero. The two are identical in effect, but the asymmetry makes a reader stop and check whether one of them is the bug — in a file whose whole point is that a silently wrong parameter integer still produces a well-formed archive. Matching the two spellings would cost nothing.
What this is
Refs #63. Deliberately not a closing keyword — criterion 8 is only partly discharged and criterion 1's second half depends on a hosted archive that does not exist. CLAUDE.md §7 is explicit that the keyword has to be right when the PR is opened, and both prior comments on #63 left it open for exactly this reason (ADR 0010's open question 3 names #63 as the owner of the cold-load measurement, and an ADR body is never edited in place). Refs #53 as well, which owns the other half.
#63 has had seven of eight criteria since #175/#176. The eighth — "Cold-load behaviour is measured and recorded in the PR: time to first painted tile on a cold cache" — could not be taken, because there was no archive, so no tile was ever painted and there was nothing to time.
map.browser.spec.tssaid so, and predicted the fix: "the day #53 lands, that test goes red — which is the right moment for somebody to come back and replace it with one that asserts tiles actually drew."This is that moment arriving from the other direction. The client's share of a cold load is ours, it does not need a host, and measuring it is what will let the hosted number — when there is one — be read as latency rather than as an unexplained total.
The defect this found
A production map fetched every tile and drew none of them, silently.
MapLibre v6 parses every vector tile in a Web Worker and locates it with
new URL('./maplibre-gl-worker.mjs', import.meta.url). Under a bundlerimport.meta.urlis the hashed chunk MapLibre was bundled into, and the expression is built from a variable, so it is not statically analysable and no bundler emits the file. The script 404s, theWorkerobject is constructed anyway, everyloadTilemessage is sent into it and never answered, and the tile sits inloadingfor ever.Every symptom is an absence. No exception,
createreturns normally, live GL context, correct origin, correct range request,registrations === 1, no maperror. The existing browser gate was green through all of it — and was right to be, because there was no tile for the worker to fail to parse.The fix is one line in
apps/web/src/map/maplibre.ts:?worker&urland not?url. The dist worker imports its siblingmaplibre-gl-shared.mjs; a verbatim copy of the one file fails on its first import and produces the same blank map by a different route.pnpm run buildnow emitsassets/maplibre-gl-worker-*.js(486 kB), referenced only from the lazy map chunk — the code split is unchanged and the entry chunk still names nothing but the chunk filename.This is a known MapLibre v6 + bundler trap rather than a discovery: see organicmaps/gtfs-osm-matcher#167, f5/unovis#921 and openwatersio/openwaters.io#122. It was reached here from the symptom, then confirmed against those.
What was built
apps/web/browser/pmtiles-fixture.tsDecompressionStreamapps/web/browser/pmtiles-fixture.test.tspmtiles' ownPMTilesclass — the classmaplibre.tshands toaddProtocol— plus an independent 40-line protobuf reader for the tile bodyapps/web/browser/harness.tswindow.__oylMapLoadapps/web/browser/map.browser.spec.tsdescribe: the render, its control, and the measurementapps/web/vite.browser.config.tsbrowser/distat build timeapps/web/vitest.config.tsbrowser/**/*.test.ts, the precedent beingpackages/fit'stools/**/*.test.tsThree decisions worth a reviewer's attention:
browser/distis already gitignored and already pruned bycheck-repo-rules.sh. A binary in the tree would need an.spdx-exemptentry §3a would refuse it, and nobody can read it in review./basemap-fixture.pmtiles, not/basemap.pmtiles. The "Publish and serve the PMTiles basemap and routing engine #53 has published nothing" test asserts the 404 at the latter and stays exactly as it was. Moving the fixture there would make a tripwire permanently green against a file of our own — a test that cannot fail.attributionfield. The product's own credit is a separate criterion and is untouched.Criterion 8 — the measurement, and what it leaves out
From the gate's own log, printed on every run (and annotated):
SharedPromiseCachecaches headers and directories only, so every visible tile is its own range request even when they all resolve to the same bytes. On a high-latency store that multiplier is the cold-load cost. A first draft of the comment asserted "two round trips, no more"; the run said three, and the comment now says what the run said.game.browser.spec.tsalready records for the shading cost, followed here.How the paint is read
preserveDrawingBufferis off in shipping code and must stay off — it is a per-frame cost paid by every rider so a test can read a pixel. The harness forces it on for its own page by patchingHTMLCanvasElement.prototype.getContextbefore the map is built.window.requestAnimationFramebefore the map exists, so the probe runs immediately after MapLibre's frame callback. Every sample came back#000000. MapLibre v6 reaches its renderer throughbrowser.frameAsync, which resolves a promise from the frame callback, so the draw happens in a microtask after anything chained synchronously onto it — the probe was reading a buffer that had already been presented and cleared.The cost of the readback is in the reported number, and is identical on the fixture page and the control page, so the two remain comparable.
Mutations — §5's gate
Each mutation was applied to a clean tree, the suite run, and the file restored byte-identically.
The fixture, against
pmtiles-fixture.test.tsring()'s command integers to the byte writer, 8448 stored as 0 and 8447 as 255, and every other assertion stayed green because the tile still parsed into the right number of vertices at the wrong placesPMTileshands the tile over, and MapLibre draws nothingsource-layermaxZoomwritten as 0 while the run length still spans the pyramidextentomitted, falling back to MapLibre's defaultThe gate, against
map.browser.spec.tssetWorkerUrl(workerUrl)removed — the production fix revertedno basemap colour reached the drawing buffer. Looked for #eeece7, #b9d4e8, #ffffff; read #f5f3ef ×10 over 300 frames. Every one of the six pre-existing browser tests stayed green, which is the measurement of why this defect survived: the old gate could not see itpreserveTheDrawingBuffer()removed#000000 ×10. This is the one that proves the control is not passing by reading nothingGates
Every command below was run on this branch at the pinned Node 24.20.0 / pnpm 11.18.0, against Chromium revision 1243 (the revision
@playwright/test1.63.0 pins).pnpm run check:reposhellcheck scripts/*.shpnpm run format:checkpnpm run lintpnpm run typecheckpnpm run test:coveragepnpm run check:a11y-suitepnpm run test:a11ybash scripts/check-a11y-suite.test.shpnpm run check:wiringbash scripts/check-wiring.test.shpnpm run check:capacitorbash scripts/check-capacitor-generated.test.shbash scripts/coverage-summary.test.shpnpm run buildpnpm run test:browserbash scripts/check-dependency-licences.test.shpnpm run check:licencesCoverage
Reported, not gated — the gate is the mutation list above.
apps/webstatementsThe denominator moves by one and the numerator does not:
setWorkerUrl(workerUrl)is a production statement that only executes in a real browser, and the browser gate is not instrumented.apps/web/browser/**is outside the coverage report'sapps/*/src/**patterns, the same waypackages/fit/tools/**is (#110) and for the same reason — a fixture generator's coverage does not belong in a client's denominator.Security
pnpm-lock.yamland everypackage.jsonare untouched;check:licencesre-run and green.?archive=and?paintDeadline=switches are harness-only (apps/web/browser/), whichvite.browser.config.tskeeps out of the shipping bundle by being a second config; the product reads its archive URL fromVITE_BASEMAP_PMTILES_URLas before.Deliberately not done
basemapStylestill emits neither, so the map draws geometry without labels. ADR 0010 has no rule for them and Publish and serve the PMTiles basemap and routing engine #53 must publish them on the basemap's own origin — the finding recorded on Render activity maps with MapLibre GL JS and self-hosted PMTiles #63 and Publish and serve the PMTiles basemap and routing engine #53 previously, unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_016X4EwacF1KuXDPhXzunifS